Highlighting: dedup field names when using wildcard expression to define which fields to highlight #10916
Conversation
…ntify which fields to highlight
javanna
added
>enhancement
v2.0.0-beta1
review
:Search/Highlighting
|
Should the fix be in simpleMatchToFullName instead? Also maybe we should leave a comment explaining that we need to deduplicate fields today but this might not be necessary anymore if we store mappings at the index level? |
good question, I was wondering myself, to be honest I have no clue :) I do have the feeling that I might be hiding some other issue with this fix, not sure. |
|
One other question could be if we should go ahead and try to highlight tons of metadata fields, like |
|
Let's ping @rjernst who might have ideas/opinions here as this is related to other refactorings he is performing. |
probably |
| @@ -76,12 +75,12 @@ public boolean hitExecutionNeeded(SearchContext context) { | |||
| public void hitExecute(SearchContext context, HitContext hitContext) { | |||
| Map<String, HighlightField> highlightFields = newHashMap(); | |||
| for (SearchContextHighlight.Field field : context.highlight().fields()) { | |||
| List<String> fieldNamesToHighlight; | |||
| Set<String> fieldNamesToHighlight; | |||
There was a problem hiding this comment.
Why not add
if (highlightFields.contains(fieldName)) { continue; }around line 94?
That'll prevent two regexes that find the same field from highlighting it twice.
There was a problem hiding this comment.
how can the same field be highlighted twice if we use a set that doesn't allow for duplicates? not sure what I'm missing here. Anyway, agree that the problem is probably in mappings code, but I am not familiar with it, somebody else will have to jump in on that.
There was a problem hiding this comment.
I'm thinking something like:
"highlight": {
"cat_*": <other stuff>,
"*": <stuff>
}As is now the last matching regex wins but it does so by highlighting the field twice. That's what I meant.
|
@rjernst any idea about whether |
|
@javanna Yes, please make the fix in |
… & `simpleMatchToIndexNames` in FieldMappersLookup Relates to elastic#10916
|
Superseded by #11377 |
… & `simpleMatchToIndexNames` in FieldMappersLookup Relates to elastic#10916 Closes elastic#11377
… & `simpleMatchToIndexNames` in FieldMappersLookup Relates to elastic#10916 Closes elastic#11377
… & `simpleMatchToIndexNames` in FieldMappersLookup Relates to elastic#10916 Closes elastic#11377
When specifying a wildcard expression to define which fields to highlight, we retrieve the matching field names from the mappers. The returned list of fields may contain duplicates, which is why we should make sure we don't go over the same field more than once. This is just to avoid performing the same task multiple times, but doesn't change the result of highlighting.